Skip to content

Comments

D/2025 03 31 side panel hover#217708

Closed
Dosant wants to merge 12 commits intoelastic:mainfrom
Dosant:d/2025-03-31-side-panel-hover
Closed

D/2025 03 31 side panel hover#217708
Dosant wants to merge 12 commits intoelastic:mainfrom
Dosant:d/2025-03-31-side-panel-hover

Conversation

@Dosant
Copy link
Contributor

@Dosant Dosant commented Apr 9, 2025

Summary

Summarize your PR. If it involves visual changes include a screenshot or gif.

Checklist

Check the PR satisfies following conditions.

Reviewers should verify this PR satisfies this list as well.

  • Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
  • Documentation was added for features that require explanation or tutorials
  • Unit or functional tests were updated or added to match the most common scenarios
  • If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
  • This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The release_note:breaking label should be applied in these situations.
  • Flaky Test Runner was used on any tests changed
  • The PR description includes the appropriate Release Notes section, and the correct release_note:* label is applied per the guidelines

Identify risks

Does this PR introduce any risks? For example, consider risks like hard to test bugs, performance regression, potential of data loss.

Describe the risk, its severity, and mitigation for each identified risk. Invite stakeholders and evaluate how to proceed before merging.

@elasticmachine
Copy link
Contributor

elasticmachine commented Apr 9, 2025

🤖 Jobs for this PR can be triggered through checkboxes. 🚧

ℹ️ To trigger the CI, please tick the checkbox below 👇

  • Click to trigger kibana-pull-request for this PR!
  • Click to trigger kibana-deploy-project-from-pr for this PR!
  • Click to trigger kibana-deploy-cloud-from-pr for this PR!

@bhavyarm bhavyarm added the ci:cloud-deploy Create or update a Cloud deployment label Apr 10, 2025
kibanamachine and others added 2 commits April 10, 2025 16:20
…de-panel-hover

# Conflicts:
#	src/platform/packages/shared/shared-ux/chrome/navigation/src/ui/components/navigation_item_open_panel.tsx
#	src/platform/test/functional/page_objects/solution_navigation.ts
#	x-pack/test/functional_solution_sidenav/tests/observability_sidenav.ts
@elasticmachine
Copy link
Contributor

elasticmachine commented Apr 11, 2025

💔 Build Failed

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #79 / ObservabilityApp o11y sidenav sidenav & breadcrumbs renders the correct nav and navigate to links
  • [job] [logs] FTR Configs #79 / ObservabilityApp o11y sidenav sidenav & breadcrumbs renders the correct nav and navigate to links
  • [job] [logs] FTR Configs #108 / serverless observability UI Observability ML Notifications page displays only notification types for observability projects
  • [job] [logs] FTR Configs #108 / serverless observability UI Observability ML Notifications page displays only notification types for observability projects
  • [job] [logs] FTR Configs #59 / serverless security UI Security ML Notifications page displays only notification types for security projects
  • [job] [logs] FTR Configs #59 / serverless security UI Security ML Notifications page displays only notification types for security projects
  • [job] [logs] FTR Configs #10 / Solution navigation smoke tests observability solution sidenav & breadcrumbs renders the correct nav and navigate to links
  • [job] [logs] FTR Configs #10 / Solution navigation smoke tests observability solution sidenav & breadcrumbs renders the correct nav and navigate to links

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
navigation 48 49 +1
serverless 43 44 +1
total +2

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
navigation 18.9KB 19.8KB +906.0B
serverless 19.6KB 20.5KB +905.0B
total +1.8KB

History

@bhavyarm
Copy link
Contributor

@Dosant / @ek-so @L1nBra and I tested this. These are our notes:

NVDA(windows):

This is how it looks in NVDA first level screen:
image (6)

Notice the list announcement here ^

On secondary nav,
When we hover on the secondary sidenav, the focus is on applications and if you press tab key, focus goes to infrastructure.

Voiceover:
First level screen:
Screenshot 2025-04-11 at 12 03 15 PM

On secondary nav,
When we hover on the secondary sidenav, the focus is on applications and if you press tab key, focus goes to infrastructure.

In both cases, the hover menu is not accessible and kinda just hanging there because mouse hover is still there.
@alexwizp is going to the code review and keep you posted. But both Lina and I would like to see the secondary nav go away when there is no focus on it.

@mgadewoll
Copy link
Contributor

mgadewoll commented Apr 14, 2025

💭 Looking at the keyboard navigation alone, it seems to work alright. The issue seem to be that keyboard + hover navigation does not quite work together as expected (I'm not sure it's a very likely scenario but we should ensure it works together anyway).
Hover closes any open sub nav and opens the current hovered one. If a user navigated with keyboard before, the focus is returned to the related button on closing of the previously open panel. This then creates the state that the opened sub nav is not accessible via keyboard navigation until it's manually closed and opened via keyboard navigation again because then the focus is moved to the first sub nav item.

If we assume hover is a "preview" of the content, it might be ok that the focus is not directly moved into the sub nav when using keyboard navigation, but what I would maybe expect is that the opened sub nav (due to hover) is not closed on onClick of the button, but instead the focus is moved into the sub nav since it's already open.
This by itself would still be confusing as it's semantically not clear the sub nav is open already due to hover. Since the buttons expand content we should in any case indicate their state (expanded) with aria-expanded.

When testing the navigation it did work in NVDA/Chrome and VO/Safari and VO/Chrome (when using VO click via control + option + Space but not with standalone Space or Enter) but it does not work with pure keyboard navigation. The sub nav always closes onClick first as it's opened due to hover.

Screen recordings
  • Chrome
Screen.Recording.2025-04-14.at.17.47.35.mov
  • NVDA/Chrome
Screen.Recording.2025-04-14.at.17.46.49.mov
  • Safari/VO
Screen.Recording.2025-04-14.at.17.51.29.mov

Additional notes

The sub nav has heading and list but the list is not labelled. When the focus is moved to the first list item, the heading is skipped so the context is not available to screen reader users. We should make sure the ul elements have an accessible label to provide context and since there are already group labels, those could be reused via aria-labelledby

@Dosant
Copy link
Contributor Author

Dosant commented Apr 15, 2025

@bhavyarm, @mgadewoll, thank you so much for your time and feedback!

If we assume hover is a "preview" of the content, it might be ok that the focus is not directly moved into the sub nav

Yes, this was our assumption with @ek-so. I can see how confusing this might be, as the "preview" and "open" panels look the same and replace each other. In the next iteration of the side navigation, we plan to visually separate these two states.

I believe that given the complexity on so many fronts regarding this intermediate hover state, we should postpone the hover feature until we have a separate "preview" panel. I will close this pull request as a "failed attempt," and we will revisit the hover feature in the next iteration with a separate preview panel.

@mgadewoll, I also understand that you pointed out at least two accessibility improvements that we should make, which are independent of hover:

  1. aria-expanded on the panel opener button
  2. label the list in the sub nav with aria-labelledby

I'll track them as bugs to fix

@Dosant Dosant closed this Apr 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci:cloud-deploy Create or update a Cloud deployment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants